[fix] Resume a gated turn under the config it was running (10/12) - #5689
[fix] Resume a gated turn under the config it was running (10/12)#5689ardaerzin wants to merge 8 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change carries effective session configuration from SDK request hydration through runner interaction records and approval resumes. It adds redaction and size limits, preserves legacy references-only behavior, and updates web approval handling to answer interactions directly. ChangesEffective configuration capture
Runner and API replay
Web approval handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SDK
participant Runner
participant InteractionStore
participant ApprovalAPI
participant MobileClient
SDK->>Runner: Send session run with effectiveParameters
Runner->>InteractionStore: Persist interaction parameters
MobileClient->>ApprovalAPI: Respond to approval interaction
ApprovalAPI->>InteractionStore: Read stored parameters
ApprovalAPI->>Runner: Resume with parameters and references
Runner-->>ApprovalAPI: Complete resumed turn
ApprovalAPI-->>MobileClient: Return interaction response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
c4025ac to
0cae609
Compare
6101ac7 to
f957a88
Compare
0cae609 to
e46be44
Compare
f957a88 to
f34d0e1
Compare
e46be44 to
9d900a8
Compare
f34d0e1 to
4223629
Compare
9d900a8 to
5321274
Compare
4223629 to
fed6233
Compare
5321274 to
1c4c784
Compare
fed6233 to
0e3c2cb
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/mobile/src/features/chat/useApprovalActions.ts (1)
69-105: 📐 Maintainability & Code Quality | 🔵 TrivialAdd unit test coverage for the
submitconflict-handling path.
approvalTargets.test.tscovers only the pureselectApprovalTargetsselector. The new logic insubmit— toleratingisInteractionConflict, countinganswered, and re-arming toidlewhen every target was already answered — is not covered by any test in this batch. This logic drives the mobile approval flow's resilience to already-answered gates.Add a test (with
queryInteractionsandrespondInteractionmocked) that exercises: all targets succeed, all targets conflict (re-arm to idle immediately), and a mixed success/conflict/non-conflict-failure case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dafe74b-d5cb-4dd4-bf47-b9e5ebb3215b
📒 Files selected for processing (39)
api/oss/src/core/sessions/interactions/dtos.pyapi/oss/src/tasks/asyncio/sessions/interactions_dispatcher.pyapi/oss/tests/pytest/unit/sessions/test_interactions_dispatcher.pydocs/design/agenta-mobile/plans/2026-07-27-mobile-approvals-steering.mddocs/design/agenta-mobile/plans/2026-07-29-effective-turn-config.mdsdks/python/agenta/sdk/agents/adapters/local.pysdks/python/agenta/sdk/agents/adapters/sandbox_agent.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/handler.pysdks/python/agenta/sdk/agents/interfaces.pysdks/python/agenta/sdk/agents/utils/effective_config.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/middlewares/running/resolver.pysdks/python/oss/tests/pytest/integration/agents/_fake_runner_backend.pysdks/python/oss/tests/pytest/unit/agents/conftest.pysdks/python/oss/tests/pytest/unit/agents/golden/run_request.pi_core.jsonsdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/test_batch_fold_stream_contract_routing.pysdks/python/oss/tests/pytest/unit/test_invoke_real_handlers_negotiation_routing.pysdks/python/oss/tests/pytest/utils/test_resolver_middleware.pyservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/protocol.tsservices/runner/src/sessions/interactions.tsservices/runner/tests/unit/interactions-parameters.test.tsservices/runner/tests/unit/wire-contract.test.tsweb/mobile/src/features/chat/approvalStamp.tsweb/mobile/src/features/chat/approvalTargets.tsweb/mobile/src/features/chat/useApprovalActions.tsweb/mobile/src/lib/auth.tsweb/mobile/tests/unit/approvalStamp.test.tsweb/mobile/tests/unit/approvalTargets.test.tsweb/packages/agenta-chat/src/transport/agentResumeRequest.tsweb/packages/agenta-chat/tests/unit/transport/agentResumeRequest.test.tsweb/packages/agenta-entities/src/session/api/api.tsweb/packages/agenta-entities/src/session/core/schema.tsweb/packages/agenta-entities/src/session/index.tsweb/packages/agenta-entities/tests/unit/session-interaction-schema.test.ts
💤 Files with no reviewable changes (3)
- web/mobile/src/lib/auth.ts
- web/mobile/tests/unit/approvalStamp.test.ts
- web/mobile/src/features/chat/approvalStamp.ts
1c4c784 to
551dc6f
Compare
0e3c2cb to
86c74ff
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
551dc6f to
28906c3
Compare
86c74ff to
ea4d80e
Compare
Railway Preview Environment
|
mmabrouk
left a comment
There was a problem hiding this comment.
🤖 The AI agent says:
@ardaerzin I found two P1 correctness issues in the mobile detached approval flow. First, approveAll can dispatch multiple resumes for one parked turn and race the warm approval session. Second, /respond can mark an approval as responded before resume dispatch is durable, which can leave the run parked with no retryable interaction. Both issues can prevent a gated turn from resuming correctly. I left the detailed findings inline.
| const reader = response.body?.getReader() | ||
| if (!reader) return | ||
| let answered = 0 | ||
| for (const row of targets) { |
There was a problem hiding this comment.
🤖 The AI agent says:
P1: Batch parallel approvals into one resume
When approveAll targets multiple gates from the same parked turn, this loop dispatches one detached resume per gate. The first resume checks the approval session out of the warm pool, so a second request arriving before it re-parks misses that session and cold-starts with only its own decision. This can duplicate replayed work or leave carried gates inconsistent. Aggregate the decisions into one resume, or wait for each partial resume to re-park before dispatching the next.
There was a problem hiding this comment.
Verified against the code rather than taken at face value, and it holds up.
compose_approval_messages builds each resume from the durable records plus the envelope for one interaction, and build_wire_messages deliberately skips interaction_request / interaction_response records as non-conversation. So a prior gate's answer is not represented anywhere in the next resume's history: resume N genuinely carries only gate N's decision. If the second dispatch misses the warm session, the cold start does not know gate A was approved.
What I have not done is fix it here, because every fix changes the respond contract rather than the client loop:
- one respond call carrying N decisions, dispatching a single resume; or
- the dispatcher folding every already-
respondedsibling gate of the same turn into each composed resume.
(2) is the more self-healing shape — whichever resume wins carries every decision made so far — but both are API design, and picking one from inside a review-fix pass seemed like the wrong call. Serialising on the client is not a real option: /respond returns once the row is CAS-flipped and the dispatch is queued, so awaiting it tells us nothing about whether the session has re-parked.
Flagging the practical scope: this only bites approveAll with more than one gate on the same parked turn. A single approval, and the per-gate buttons, dispatch one resume and are unaffected.
Happy to take this as a follow-up with a concrete proposal for (2) — I would rather do that than bolt a partial fix onto the client loop.
| } catch { | ||
| // Connection dropped (screen locked, network change) — the run | ||
| // continues server-side; records polling picks the result up. | ||
| await respondInteraction({ |
There was a problem hiding this comment.
🤖 The AI agent says:
P1: Keep failed approval dispatches retryable
When /respond successfully CAS-transitions the row to responded but broker enqueueing fails, or the queued worker exhausts its retries, the interaction is no longer actionable even though no resume occurred. This client path then cannot retry: the next query omits the row, and a direct retry receives the conflict treated as benign below, leaving the run parked indefinitely. The status transition needs durable dispatch or outbox semantics, or a recoverable state, before mobile relies on it.
There was a problem hiding this comment.
Agreed on the mechanism: /respond CAS-flips the row to responded and enqueues separately, so a broker failure or an exhausted worker retry leaves a gate that is no longer actionable and no resume that ever ran. The next actionable_only query omits the row, and a direct retry hits the conflict this client treats as benign, so the run stays parked.
I am not fixing this in this pass, deliberately. Durable dispatch or outbox semantics is a change to what responded means and to the lifecycle every consumer of these rows reads — the desktop inbox, the mobile inbox, the reconciliation sweep in the records worker. That is a design decision about the interactions plane, not a defect in the mobile client, and doing it inside a review-fix pass would land the least-reviewed version of it.
Two things that bound the exposure meanwhile, for whoever picks this up:
- The window is broker-enqueue failure or terminal worker failure, not ordinary transport error. A
/respondthat never reaches the API leaves the rowpendingand fully retryable. - A stuck row is recoverable out of band today: the gate can still be cancelled, and a new turn's start sweep clears prior-turn gates.
Worth a ticket against the interactions plane with the recovery semantics decided up front — a responded row that no resume claimed within a bound should become actionable again, which is the outbox in its smallest useful form.
e062f59 to
22a1bbb
Compare
d13945a to
247651d
Compare
22a1bbb to
c0d4795
Compare
247651d to
4233737
Compare
4233737 to
0c1808e
Compare
878e344 to
455ac6e
Compare
0c1808e to
3b92ff2
Compare
455ac6e to
c3227f8
Compare
3b92ff2 to
88c741d
Compare
c3227f8 to
9f6a957
Compare
88c741d to
5997eb1
Compare
A references-only /invoke (no data.parameters, no data.revision) never hydrated its references, so the run silently used the service's registered default configuration instead of the referenced revision's. This is the mobile resume path; desktop was unaffected because it always sends inline data.parameters. The hydration gate consulted the revision returned by resolve_revision, which falls back to RunningContext.revision — pre-seeded by the decorator with the registered default config. That revision is always populated for the agent builtin, so the gate read "already configured" and skipped hydration. seed_empty_parameters_from_configuration also ran before the gate, so it could never have observed empty parameters anyway. Decide hydration purely from caller-supplied config (data.parameters or data.revision), and seed the registered default only after a hydration attempt has been made and come back empty, so it stays a fallback rather than a pre-emption.
Grounded in two live experiments on the EE dev stack: - A dirty-config run's session_interactions row carries only workflow + workflow_variant references (no workflow_revision), so a references-only resume hydrates the variant HEAD, not the draft the turn ran under. A committed run's revision reference is pinned and is therefore already immune to later commits. - A warm approval resume keeps the sandbox's acquire-time model and secrets while re-reading the permission map from the incoming (hydrated, committed) request -- a split brain where the approval UI enforced one policy and the resumed turn enforces another. Cold replay runs the committed config end to end against a draft transcript. Recommends stamping the effective parameters on the interaction row at gate creation (SDK emits, runner echoes, API replays inline), which needs no migration, with a detect-and-defer fallback for pre-change rows.
A HITL gate answered from a client that cannot reproduce the turn's config (mobile, the M2 dispatcher) resumes references-only, so the SDK hydrates the referenced variant's HEAD revision instead of what the gated turn was running - wrong model, wrong instructions, and wrong tool permissions on a dirty run. The SDK is the only component holding the post-hydration config, so it emits it: the handler carries its resolved data.parameters onto SessionConfig, and request_to_wire puts it on the wire as the opaque effectiveParameters. Emitted only for a session run (nothing else can park a gate), so an ad-hoc run's payload stays byte-identical to the golden contract. Redacted (an MCP connection's static headers are the one place the schema permits a raw credential; the vault-key refs survive) and capped at 64 KB, dropped whole with a log line rather than truncated into invalid JSON. The runner echoes the blob onto the interaction row; the answering client replays it as data.parameters, which suppresses hydration and reproduces the turn.
The runner writes the durable row at the exact moment a gate parks, so it is where the
turn's config has to be recorded - but it had no access to it (there was no parameters
field on the /run wire at all). The SDK now stamps effectiveParameters; this echoes it
verbatim into the row's data.parameters, alongside data.request and data.references.
Opaque by design: the runner never reads inside the blob and derives no behavior from it.
It is deliberately NOT in configFingerprint - that hash decides warm resume vs cold replay,
and the blob is a projection of fields already hashed, so including it would let a cosmetic
serialization change evict every warm session. A turn with no stamped config omits the KEY
(not null, not {}): a legacy row keeps its exact shape, and an empty inline config would
still suppress hydration server-side and resume a toolless agent.
Two links in the same chain. The DTO: SessionInteractionData is a closed pydantic model with the default extra="ignore", and the postgres mapping round-trips through it on write and on read even though data is a schemaless json column - so the runner's new data.parameters was being dropped twice with no error anywhere. Declaring the field is the whole fix; no migration. The dispatcher: when the row carries a config, send it inline on the resume. The SDK resolver decides hydration purely from what the caller sent, so inline parameters suppress it and the run continues under the config the gate was raised against instead of the referenced variant's HEAD revision. References still ride along as attribution. A row written before the runner stamped configs has none and produces the byte-identical references-only body this dispatcher has always sent.
Records what landed on the backend lanes of the effective-turn-config plan and notes in the approvals round-trip that a pre-change row (or one over the stamp cap) is still answerable, degrading to reference hydration.
The lite resume answered a HITL gate with a references-only body, so the SDK
hydrated the referenced variant's HEAD revision. For a dirty run that is a
different model and — the security-relevant half — a different tool-permission
map than the gate was approved under.
The runner now stamps the turn's post-hydration config onto the interaction row
as data.parameters. Read it off the same row the references come from and send
it inline, which suppresses hydration and reproduces the turn exactly.
Emit the key ONLY when the stamped config is a non-empty object: an empty {}
also suppresses hydration and would run an unconfigured agent. Rows without it
(legacy, over-cap, pre-stamping runner) keep today's references-only path.
Declare parameters on the interaction zod schema too — objects strip unknown
keys by default, so an undeclared field would be silently dropped and the fix
would no-op with everything green.
The phone built its own /invoke resume from a stamped records replay. That lands as a
NEW turn (the keepalive trips approval-mismatch (history) -> evict + cold), so the parked
gate is never matched and the interaction row stays pending — the desktop keeps showing
"Approval needed to continue" even after the tool ran.
Call POST /sessions/interactions/{id}/respond instead: the backend CAS-flips the row to
responded and the interactions worker rebuilds the history server-side and replays the
gate's stamped effective config, so the resume lands warm. Approve-all fans out one
respond per pending gate; a 409 (already answered) settles to idle instead of erroring.
respondInteraction now throws instead of swallowing the failure — a mutation's caller has
to tell a real failure from an already-answered gate (isInteractionConflict).
Drops the mobile-only invoke plumbing: approvalStamp.ts and the invoke bearer header.
9f6a957 to
ed21782
Compare
5997eb1 to
3561f72
Compare
Context
An approval answered from a phone could resume the turn under the wrong configuration. The gate carried references to the workflow, so the resume hydrated the referenced variant's HEAD revision rather than the config the paused turn was actually running. If the agent had been invoked with a draft config, or the variant moved on, the resumed turn ran as a different agent than the one you approved.
This is one contract across three tiers, which is why it is one PR: splitting it would leave a wire field written by nobody or read by nobody.
Changes
The SDK stamps the post-hydration config it is about to run onto the
/runwire aseffectiveParameters(session runs only, credential-stripped, capped at 64KB). The runner echoes it onto the interaction row of any gate that turn parks. The API replays it as the resume'sdata.parameters, which suppresses reference hydration server-side and reproduces the turn, tool permissions included.Also fixed here: the SDK hydrated references even when the caller had sent an inline config, so a caller-supplied configuration was silently discarded. Hydration is now decided from what the caller actually sent.
Tests / notes
protocol.tsandwire.pyand pinned by shared golden fixtures; both sides' contract tests are updated together.parametersand resume via references exactly as before.